added Endianness access from IFD to python #106
Conversation
feefladder
force-pushed
the
endianness-access
branch
from
4dfe3b4 to
9b17ba4
Compare
|
You can see my preferred way of implementing pyo3 enums here:
|
feefladder
force-pushed
the
endianness-access
branch
2 times, most recently
from
b4b8261 to
6a4faee
Compare
feefladder
force-pushed
the
endianness-access
branch
from
6a4faee to
0ca7e74
Compare
| #[getter] | ||
| pub fn endianness(&self) -> PyEndianness { | ||
| self.0.endianness().into() | ||
| } |
There was a problem hiding this comment.
Also add this to the pyi file with the ImageFileDirectory type hint.
There was a problem hiding this comment.
I think you mean Endianness type hint?
There was a problem hiding this comment.
No I mean you need to describe this here:
There was a problem hiding this comment.
I did that (lines 15-16, also comment). What I meant is a -> Endianness type hint on the declaration
python/src/enums.rs
Outdated
| use async_tiff::{ | ||
| reader::Endianness, | ||
| tiff::tags::{ | ||
| CompressionMethod, PhotometricInterpretation, PlanarConfiguration, Predictor, | ||
| ResolutionUnit, SampleFormat, | ||
| }, |
There was a problem hiding this comment.
Can we keep this as module imports instead of crate imports.
python/python/async_tiff/enums.py
Outdated
| return str(self.value) | ||
|
|
||
| class Endianness(StrEnum): |
There was a problem hiding this comment.
We need to add Python formatting to CI; as this would fail black/ruff fmt.
There was a problem hiding this comment.
I see you opened an issue for that. I could pull it into this PR though?
There was a problem hiding this comment.
Actually, I don't know how to do that...
There was a problem hiding this comment.
I'd prefer a separate PR anyways. You can look at https://github.com/developmentseed/obstore/blob/ec7dd298f62bf48a23dd677d3780540ece883bfb/.pre-commit-config.yaml#L20-L25 and https://github.com/developmentseed/obstore/blob/ec7dd298f62bf48a23dd677d3780540ece883bfb/.github/workflows/test-python.yml#L17-L39
feefladder
force-pushed
the
endianness-access
branch
from
605f233 to
b900921
Compare
feefladder
force-pushed
the
endianness-access
branch
from
b900921 to
d63e299
Compare
| @property | ||
| def endianness(self) -> Endianness: ... |
|
|
||
| @property | ||
| def x(self) -> int: | ||
| """The column index this tile represents.""" | ||
|
|
||
| @property | ||
| def y(self) -> int: | ||
| """The row index this tile represents.""" | ||
|
|
||
| @property | ||
| def compressed_bytes(self) -> Buffer: | ||
| """The compressed bytes underlying this tile.""" | ||
|
|
||
| @property | ||
| def compression_method(self) -> CompressionMethod | int: | ||
| """The compression method used by this tile.""" | ||
|
|
There was a problem hiding this comment.
black added these
There was a problem hiding this comment.
For all intents and purposes black and ruff format are the same, but they have some slight differences. Black is the older tool and ruff format is the preferred newer tool. Sorry for the confusion but we want to be using ruff format
There was a problem hiding this comment.
In any case, I'd prefer for ruff format to be added as a separate PR with CI validation first, just like the cargo import ordering
closes #91 No unit tests yet. There were none before me in the Python part. Would be happy to add some with some pointers.
I chose to duplicate the enum in Python, so users can access it as
Endianness.LittleEndianin Python as well, using pyo3 macro derivation.I added the repr(u16) and custom discriminant on
PyEndiannessfor matching tiff structure. I could reflect that in src/reader.rs::Endiannessor drop completely.It seems that
FromPyObjecttrait impl is needed if we want to pass it into a rust function from the python side. Since we don't currently do that, I didn't implement it. Could be nicely done to accept all ofEndianness,u16orStringI think?I'm a bit unsure of the location of
PyEndiannessSince its an enum I put it in python/src/enums.rs, even though all enums there are tags. On the rust sideEndiannessis inreader.rs, which makes sense implementation-wise I think? Should it actually be inpython/src/reader.rs? Or should we wait for repo layout (#69).